BFDD : BFD session stays until the last static route over nexhop #19031

varuntumbe · 2025-06-15T10:16:33Z

What is the problem ?

When we have more then one static route over the same nexthop with BFD, session goes down and gets cleared if we remove one of the static route.

What is the Root Cause ?

When we create static route over a nexthop with BFD, bfdd_dest_register routine will be called where for the first flow, we create new pcn object ( pcn_new ), we increment the refcount and notify the client via ptm_bfd_notify

But for the subsequent addition of static route over the same nh, means pcn_lookup would give the same existing pcn skipping the increment of refcount.

so essentially, refcount remains at value 1, even if you add any number of static route over the nh.

When we remove one of the static, bfdd_dest_deregister will called where we do pcn_free ( where we decrement the refcount ) and session delete ( _ptm_bfd_session_del )

What is the fix ?

Fix has 2 parts

Incrementing the refcount properly ( early before pcn_lookup) for every static route addition. This makes sures that refcount gets updated properly for every addition of static route
Now when we start removing the static routes one by one, we decrement the refcount ( in bfdd_dest_deregister ) and will call the pcn_free only when refcount becomes 0 ( essentially deleting the session only when there are no static routes points to nh )

Closes #19014

ton31337

Please update the commit also (instead of TBD).

ton31337 · 2025-06-16T12:19:55Z

bfdd/ptm_adapter.c

 	 * and the worst case is the refcount will detain us.
 	 */
-	_ptm_bfd_session_del(bs, BD_NEIGHBOR_DOWN);
+	//_ptm_bfd_session_del(bs, BD_NEIGHBOR_DOWN);


Here,

This func call ( _ptm_bfd_session_del ) looks like it has been added for a specific scenario rather to address specific bug, not sure how to handle this here.

Basically this call should be deleted, why ?

As we remove the static routes one by one, refcount gets decremented one by one and once it reaches 0, we call the pcn_free where we clean up the pcn related data structures and call _ptm_bfd_session_del where it notifies the peer about the session down and frees up BFD session related data structures.

So according to my opinion, this call is not needed.

Then let's drop it instead of commenting.

ton31337 · 2025-06-16T12:20:26Z

bfdd/ptm_adapter.c

 					       struct bfd_session *bs)
 {
 	struct ptm_client_notification *pcn;
+	/* Lets increment the refcount in the beginning itself regardless of its a new pcn or old*/


What is the difference here or how was it before?

Earlier flow :
bfdd_dest_register
pcn_new
==> here we try to find whether there is already a existing pcn for the particular bsd session, if found, we used to return without updating the ref_count ( refcount updation was later pcn_lookup ) which mess up the refcount logic.

===> if we add multiple static route over the same bfd peer nh, for the subsequent addition, pcn_lookup will give the existing pcn which result in NOT updating the refcount

Now refcount incrementation has been added before the pcn_lookup which makes the refcount increment properly

ton31337 · 2025-06-16T12:20:54Z

tests/topotests/bfd_static_topo/rt1/frr.conf

@@ -0,0 +1,29 @@
+hostname rt1
+log file /tmp/topotests/bfd_ospf_topo1.test_bfd_ospf_topo1/rt1/mgmtd.log


Drop every log file statement.

ton31337 · 2025-06-16T12:21:04Z

tests/topotests/bfd_static_topo/rt1/frr.conf

+log file /tmp/topotests/bfd_ospf_topo1.test_bfd_ospf_topo1/rt1/bfdd.log
+no service integrated-vtysh-config
+!
+password 1


Drop the password statement.

ton31337 · 2025-06-16T12:21:10Z

tests/topotests/bfd_static_topo/rt1/frr.conf

+log file /tmp/topotests/bfd_ospf_topo1.test_bfd_ospf_topo1/rt1/ospf6d.log
+log file /tmp/topotests/bfd_ospf_topo1.test_bfd_ospf_topo1/rt1/staticd.log
+log file /tmp/topotests/bfd_ospf_topo1.test_bfd_ospf_topo1/rt1/bfdd.log
+no service integrated-vtysh-config


Why this is needed?

ton31337 · 2025-06-16T12:21:27Z

tests/topotests/bfd_static_topo/rt1/frr.conf

+!
+password 1
+!
+ip route 1.1.1.2/32 10.0.1.2 bfd


No Cloudflare IPs, please. Use private ranges or documentation ranges.

ton31337 · 2025-06-16T12:21:51Z

tests/topotests/bfd_static_topo/rt2/frr.conf

+exit
+!
+interface lo
+ ip address 2.2.2.2/32


Please, use private or documentation ranges.

ton31337 · 2025-06-16T12:22:05Z

tests/topotests/bfd_static_topo/test_bfd_static_topo1.py

+# SPDX-License-Identifier: ISC
+
+#
+# test_bfd_static_vrf.py


Bad naming.

Changed the name

ton31337 · 2025-06-16T12:22:31Z

tests/topotests/bfd_static_topo/test_bfd_static_topo1.py

+# test_bfd_static_vrf.py
+# Part of NetDEF Topology Tests
+#
+# Copyright 2025 6WIND S.A.


Are you behind 6wind?

Changed the copyright

…utes over same nh Adding the topotest which verifies whether BFD session stays up as long as there is one static route pointing to nexthop bfd peer Signed-off-by: Varun Hegde <[email protected]>

This Fix makes the BFD session to stay up until the last static route going over the gateway ip which is tracked by bfd. Signed-off-by: Varun Hegde <[email protected]>

varuntumbe · 2025-06-17T17:12:05Z

======> cmd : show bfd peers json

[
{
"multihop":false,
"peer":"2001:db8:1::1",
"local":"2001:db8:1::2",
"vrf":"default",
"interface":"r2-eth0",
"id":2411218638,
"remote-id":38497228,
"passive-mode":false,
"log-session-changes":false,
"status":"up",
"uptime":4,
"diagnostic":"ok",
"remote-diagnostic":"ok",
"type":"dynamic",
"receive-interval":600,
"transmit-interval":600,
"echo-receive-interval":50,
"echo-transmit-interval":0,
"detect-multiplier":3,
"remote-receive-interval":600,
"remote-transmit-interval":600,
"remote-echo-receive-interval":50,
"remote-detect-multiplier":3,
"rtt-min":0,
"rtt-avg":0,
"rtt-max":0
},
{
"multihop":false,
"peer":"2001:db8:2::1",
"vrf":"default",
"id":3088557958,
"remote-id":2847372379,
"passive-mode":false,
"log-session-changes":false,
"status":"up",
"uptime":3,
"diagnostic":"ok",
"remote-diagnostic":"ok",
"type":"dynamic",
"receive-interval":2000,
"transmit-interval":2000,
"echo-receive-interval":50,
"echo-transmit-interval":0,
"detect-multiplier":3,
"remote-receive-interval":2000,
"remote-transmit-interval":2000,
"remote-echo-receive-interval":50,
"remote-detect-multiplier":3,
"rtt-min":0,
"rtt-avg":0,
"rtt-max":0
}
]

================> from the router

[
{
"detect-multiplier": 3,
"diagnostic": "ok",
"echo-receive-interval": 50,
"echo-transmit-interval": 0,
"id": "",
"interface": "r2-eth0",
"local": "2001:db8:1::2",
"multihop": false,
"passive-mode": false,
"peer": "2001:db8:1::1",
"receive-interval": 600,
"remote-detect-multiplier": 3,
"remote-diagnostic": "ok",
"remote-echo-receive-interval": 50,
"remote-id": "",
"remote-receive-interval": 600,
"remote-transmit-interval": 600,
"status": "up",
"uptime": "",
"transmit-interval": 600
},
{
"detect-multiplier": 3,
"diagnostic": "ok",
"echo-receive-interval": 50,
"echo-transmit-interval": 0,
"id": "",
"interface": "r2-eth1",
"local": "2001:db8:2::2",
"multihop": false,
"passive-mode": false,
"peer": "2001:db8:2::1",
"receive-interval": 2000,
"remote-detect-multiplier": 3,
"remote-diagnostic": "ok",
"remote-echo-receive-interval": 50,
"remote-id": "",
"remote-receive-interval": 2000,
"remote-transmit-interval": 2000,
"status": "up",
"uptime": "",
"transmit-interval": 2000
}
]

===============> expected o/p from the testcase file

one of the topotest failing consistantly ( bfd_topo3 ) where expected does not match the received o/p from the router.

When I checked, its failing because its not able to find "local" and/or "interface" fields ( which are in expected files but not in router gen o/p )

I dont see how my changes affected this failure. Any idea @ton31337 ?

Thanks,
Varun

ton31337 · 2025-06-20T07:14:45Z

tests/topotests/bfd_static_topo/rt1/frr.conf

+!
+
+interface lo
+ ip address 1.1.1.1/32


No Cloudflare, please, use private or documentation ranges.

ton31337 · 2025-06-20T07:15:09Z

tests/topotests/bfd_static_topo/rt2/frr.conf

+exit
+!
+interface lo
+ ip address 2.2.2.2/32


Please, use private or documentation ranges.

ton31337 · 2025-06-20T07:15:30Z

tests/topotests/bfd_static_topo/test_bfd_static_topo1.py

+#
+# test_bfd_static_topo1.py
+#
+# Copyright (c) 2024 by Varun Hegde


My calendar now shows it's 2025.

ton31337 · 2025-06-20T07:20:47Z

bfdd/ptm_adapter.c

 	 * and the worst case is the refcount will detain us.
 	 */
-	_ptm_bfd_session_del(bs, BD_NEIGHBOR_DOWN);
+	//_ptm_bfd_session_del(bs, BD_NEIGHBOR_DOWN);


Then let's drop it instead of commenting.

thierrydevriendt · 2025-11-17T14:01:32Z

Hello,

Could you let me know when this fix can be merged?

Thanks.

ton31337 · 2025-11-21T13:12:38Z

@varuntumbe are you still working on this?

varuntumbe · 2025-11-23T16:37:09Z

@varuntumbe are you still working on this?

Hi @ton31337 ,

This solution is fundamentally wrong ( as I found out when one of the bfd topo test failed ). This needs a rework. I tried reworking this couple of months back but was not able to code it up to a proper solution. I am not working on this now. Apologies.

thanks

frrbot bot added the bfd label Jun 15, 2025

github-actions bot added master size/L rebase PR needs rebase labels Jun 15, 2025

ton31337 requested changes Jun 16, 2025

View reviewed changes

varuntumbe force-pushed the dev/bfd_static_route_fix branch from c2bc60d to 88e9541 Compare June 17, 2025 16:28

varuntumbe added 2 commits June 17, 2025 22:03

topotests: Adding a test to determine the BFD behavior with static ro…

8ab2790

…utes over same nh Adding the topotest which verifies whether BFD session stays up as long as there is one static route pointing to nexthop bfd peer Signed-off-by: Varun Hegde <[email protected]>

bfdd: Making the BFD session stay up until the last static route

93ed0ce

This Fix makes the BFD session to stay up until the last static route going over the gateway ip which is tracked by bfd. Signed-off-by: Varun Hegde <[email protected]>

varuntumbe force-pushed the dev/bfd_static_route_fix branch from 88e9541 to 93ed0ce Compare June 17, 2025 16:34

ton31337 reviewed Jun 20, 2025

View reviewed changes

		@@ -0,0 +1,29 @@
		hostname rt1
		log file /tmp/topotests/bfd_ospf_topo1.test_bfd_ospf_topo1/rt1/mgmtd.log

BFDD : BFD session stays until the last static route over nexhop #19031

Are you sure you want to change the base?

BFDD : BFD session stays until the last static route over nexhop #19031

Uh oh!

Conversation

varuntumbe commented Jun 15, 2025 • edited by ton31337 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ton31337 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

varuntumbe commented Jun 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thierrydevriendt commented Nov 17, 2025

Uh oh!

ton31337 commented Nov 21, 2025

Uh oh!

varuntumbe commented Nov 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

varuntumbe commented Jun 15, 2025 •

edited by ton31337

Loading